src/rbac/user.rs (1)

72-76: Align OAuth userid fallback with email as secondary

To stay consistent with the OIDC login fallback (name → email → sub), consider including email in the userid fallback chain. While sub should be present, this makes the constructor resilient and consistent with reply_login.

-                userid: user_info
-                    .sub
-                    .clone()
-                    .or_else(|| user_info.name.clone())
-                    .unwrap_or(username),
+                userid: user_info
+                    .sub
+                    .clone()
+                    .or_else(|| user_info.name.clone())
+                    .or_else(|| user_info.email.clone())
+                    .unwrap_or(username),

src/handlers/http/oidc.rs (2)

244-264: User lookup: consider preferred_username as an additional fallback

Great migration helper. For completeness, also try preferred_username between name and email.

     if let Some(name) = &user_info.name {
         if let Some(user) = Users.get_user(name) {
             return Some(user);
         }
     }

+    if let Some(preferred) = &user_info.preferred_username {
+        if let Some(user) = Users.get_user(preferred) {
+            return Some(user);
+        }
+    }
+
     if let Some(email) = &user_info.email {
         if let Some(user) = Users.get_user(email) {
             return Some(user);
         }
     }

---

335-341: Harden user_id cookie: set HttpOnly (and optionally Secure)

Unless the frontend must read the user_id, make it HttpOnly to reduce exposure. Consider Secure in production.

 pub fn cookie_userid(user_id: &str) -> Cookie<'static> {
     Cookie::build(USER_ID_COOKIE_NAME, user_id.to_string())
         .max_age(time::Duration::days(COOKIE_AGE_DAYS as i64))
         .same_site(SameSite::Strict)
+        .http_only(true)
+        // .secure(true) // consider enabling under HTTPS
         .path("/")
         .finish()
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.858Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.

src/handlers/http/oidc.rs (2)

167-176: Avoid panics on missing claims; derive user_id robustly using ID token sub

.expect(...) will panic if a provider omits optional fields in userinfo. Prefer falling back to claims.sub (required in ID Token) and return a controlled error if ever absent.

Apply:

-    let username = user_info
-        .name
-        .clone()
-        .or_else(|| user_info.email.clone())
-        .or_else(|| user_info.sub.clone())
-        .expect("OIDC provider did not return a usable identifier (name, email or sub)");
-    let user_id = user_info
-        .sub
-        .clone()
-        .expect("OIDC provider did not return a usable identifier (sub)");
+    let username = user_info
+        .name
+        .clone()
+        .or_else(|| user_info.email.clone())
+        // fall back to ID Token `sub` (required by spec)
+        .unwrap_or_else(|| claims.sub.clone());
+    let user_id = user_info
+        .sub
+        .clone()
+        .unwrap_or_else(|| claims.sub.clone());

---

390-416: When migrating to sub, also update group memberships and stale sessions

You migrate the stored user object and persist it, but references in metadata.user_groups still point to the old id; stale sessions may also point to the old id. Update both during migration.

Augment the metadata update:

-    oauth_user.user_info.clone_from(&user_info);
+    oauth_user.user_info.clone_from(&user_info);
     *roles = group;
 
     // Update userid to use sub if available (migration from name-based to sub-based identification)
     if let Some(ref sub) = user_info.sub {
         oauth_user.userid.clone_from(sub);
     }
@@
-    if let Some(entry) = metadata
+    if let Some(entry) = metadata
         .users
         .iter_mut()
         .find(|x| x.username() == old_username)
     {
         entry.clone_from(&user);
+        // Also migrate user references inside user groups
+        for grp in metadata.user_groups.iter_mut() {
+            if grp.users.remove(&old_username) {
+                grp.users.insert(user.username().to_string());
+            }
+        }
         put_metadata(&metadata).await?;
     }

For sessions: if the in-memory session map stores the old id, either:

• Migrate session owner ids from old_username → user.username(), or
• Invalidate sessions for old_username to avoid orphaned sessions.

If you want, I can draft a small API addition in Users to migrate or purge sessions for a given user id.

src/handlers/http/oidc.rs (1)

195-201: Handle poisoned DEFAULT_ROLE lock gracefully

lock().unwrap() can panic if the mutex is poisoned. Fall back safely.

-    let default_role = if let Some(default_role) = DEFAULT_ROLE.lock().unwrap().clone() {
-        HashSet::from([default_role])
-    } else {
-        HashSet::new()
-    };
+    let default_role = match DEFAULT_ROLE.lock() {
+        Ok(guard) => guard
+            .clone()
+            .map(|role| HashSet::from([role]))
+            .unwrap_or_default(),
+        Err(_) => HashSet::new(),
+    };

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.858Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.